Add security context at pod and container level for Kanister operator #3075
Conversation
julio-lopez
changed the title
anishbista60
force-pushed
the
security-context
branch
from
6836d0d to
5d30468
Compare
|
@julio-lopez please test it . |
|
@viveksinghggits could you please look into this PR sir ? |
Can you please add the test plan. We have a helm test written here Also, you don't have to say sir, just Vivek is fine. |
Usually when someone raises a PR it's their responsibility to add proper test plan and get help to test the changes. Like suggested above, let's add some tests and we can try to merge. |
yes sure. Thank you |
anishbista60
force-pushed
the
security-context
branch
3 times, most recently
from
57f61ca to
9d9ccd1
Compare
|
@viveksinghggits could you please take a look ? |
anishbista60
force-pushed
the
security-context
branch
from
9d9ccd1 to
7ce7434
Compare
anishbista60
force-pushed
the
security-context
branch
from
7ce7434 to
fe1a4b7
Compare
|
@viveksinghggits Thanks for your review. I think now it's look fine according to your recommendation. |
anishbista60
force-pushed
the
security-context
branch
2 times, most recently
from
1ff8179 to
3aeb47d
Compare
anishbista60
force-pushed
the
security-context
branch
2 times, most recently
from
f06c4b4 to
d207e06
Compare
|
@viveksinghggits shall i go for removing that 2nd and 3 rd test cases ? |
There was a problem hiding this comment.
@anishbista60 so where we commit/push files to github, we should have a newline at the end of the file. It's general feedback for the files in this PR. Can you please make sure that they are committed properly so that they have the new line at the end.
anishbista60
force-pushed
the
security-context
branch
2 times, most recently
from
5f5302a to
3b4c154
Compare
|
now , i think looks good . Could you please reply on above in unresolved review ? |
|
Anyone here please take a look and try to get it merge. |
| @@ -89,3 +89,4 @@ tolerations: [] | |||
| # | |||
| # node labels for pod assignment. Evaluated as template | |||
| nodeSelector: {} | |||
|
|
|||
There was a problem hiding this comment.
are we not adding any new fields in values.yaml for .Values.controller.containerSecurityContext? Is it already part of the values.yaml?
There was a problem hiding this comment.
@viveksinghggits no we are not adding . Please check this discussion
and regarding for nodeSelector . It was already there before this PR. i added the new line based on this recommendation
There was a problem hiding this comment.
@anishbista60 I think there was a miscommunication. I was not talking about removing entire helm field. I was talking about removing the things that you had commented out #3075 (comment) and removing the default values that you had put. We would still the new helm fields that the PR is introducing.
…erator Signed-off-by: Anish Bista <[email protected]>
Signed-off-by: Anish Bista <[email protected]>
Signed-off-by: Anish Bista <[email protected]>
anishbista60
force-pushed
the
security-context
branch
from
a9f1c2a to
7831ed4
Compare
Change Overview
Expose securityContext on kanister-operator deployment to improve security
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
make helm-testto verify that helm chart is working fine.